Add security foundations section #487
Conversation
|
No New Or Fixed Issues Found |
Deploying contributing-docs with
|
| Latest commit: |
528330f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8a130d8c.contributing-docs.pages.dev |
| Branch Preview URL: | https://ps-add-fundamental-reqs.contributing-docs.pages.dev |
| The requirements in this section are organized hierarchically, with top-level requirements defining | ||
| the core rules and obligations that must be met, serving as broad objectives for Bitwarden's | ||
| security model. Sub-requirements expand on these by addressing specific scenarios, exceptions, or | ||
| clarifications, and may override their parent requirement when explicitly stated. Sibling | ||
| requirements at the same level must remain independent and free of contradictions. |
There was a problem hiding this comment.
thought: Might be nice to align this to an industry standard like ISO/IEC/IEEE 29148, but there's only so much we can do in a markdown document
| 2. Vault data **MAY** be unprotected while _in use_. | ||
|
|
||
| - a. The Client **MAY** decrypt all vault data during vault unlock. | ||
| - i. The Client **SHOULD** minimize the quantity of decrypted vault data. |
There was a problem hiding this comment.
thought: we could make this more stringent
| - i. The Client **SHOULD** minimize the quantity of decrypted vault data. | |
| - i. The Client **SHOULD** limit decrypted data to the records actively in use by the user. |
| @@ -0,0 +1,21 @@ | |||
| # P03 - No security on fully compromised systems | |||
There was a problem hiding this comment.
point-of-interest: this was previously called No guarantees for unlocked vaults on fully compromised devices. I changed it to make it more consistent with the other principles, but "no security" might seems a bit harsh
There was a problem hiding this comment.
Related to the confluence point; I think we might need to add "no security guarantees on fully compromised bitwarden contexts"
|
Thoughts on dropping the acronyms and typed declarations? "P01" and "EK" come off rather strongly to me. Same for the "MUST" and "MUST NOT" all over the place that could probably just be expressed without capitalization and boldface. |
|
@withinfocus for The boldface might be distracting you're probably right, but I'm not sure about removing the capitalization. Here I'm drawing inspiration from W3C specifications such as WebAuthn L3 where those terms are always captialized |
|
I get where you're coming from with the historical references, but I see those as specifications with a large scope and impact. This is important to the company as principles, but I wouldn't treat it quite so intensely such as to model it vs. writing natural language. |
|
Imo: I prefer the RFC-like MUST NOT, and SHOULD NOT, etc. because it makes it clear that we are unambiguous and very deliberate about whatever meaning we encode with them, because they have a clear definition (from rfc2119). So, if we want to treat this as a set of requirements, that we want to design our applications by, then the RFC-style terms helps make these requirements unambiguous and clear. |
|
My vision was that these requirements would be what we base all of our security design on, and not be "mere guidelines". If that will happen or not is probably beyond the scope of this PR. :) That said, the primary reason that motivated me to create this documentation was to remove ambiguousness surrounding security design and make it very clear for teams that have to design solutions for our products. Given that I am hesitant to make any changes that could potentially decrease clarity and introduce ambiguousness |
docs/security/definitions.mdx
Outdated
| <dt>User</dt> | ||
| <dd>The owner of the vault data.</dd> |
There was a problem hiding this comment.
-
Does a User include an organization?
-
Who is the owner of vault data? For example - organizations often consider themselves the "owner" of data stored in an individual user's vault if it's a business account. But we generally mean "the User or Organization linked to the cipher in the database". This will increase as we give organizations more control over individual accounts (e.g. password reset, delete).
There was a problem hiding this comment.
I tweaked this a little bit to avoid the question of ownership
|
|
||
| - a. The Client **MUST** encrypt vault data stored on disk. | ||
| - b. The Client **MUST** use the UserKey to encrypt vault data stored on the Server. | ||
| - c. The Client **SHOULD** ensure that no mechanisms exists such that vault data may be stored to |
There was a problem hiding this comment.
typo:
| - c. The Client **SHOULD** ensure that no mechanisms exists such that vault data may be stored to | |
| - c. The Client **SHOULD** ensure that no mechanisms exist such that vault data may be stored to |
| - a. The Client **MAY** decrypt all vault data during vault unlock. | ||
| - i. The Client **SHOULD** minimize the quantity of decrypted vault data. | ||
| - b. The Client **MAY** leave vault data encrypted after unlock. | ||
| - c. The Client **MUST** ensure that unprotected data does not linger when no longer _in use_. |
There was a problem hiding this comment.
What do we mean by linger? This is all very precise but "linger" is quite vague.
There was a problem hiding this comment.
Hmmm, I think is not present in memory is what I'm looking for here, but that makes this requirement hard to implement. Maybe we should change it to a SHOULD at the same time
| - c. The Client **MUST** ensure that unprotected data does not linger when no longer _in use_. | |
| - c. The Client **SHOULD** ensure that unprotected data is not present in memory when no longer _in use_. |
I still think this requirement will have a pretty large impact if adopted since it tends to discourage our huge caches, but having no caches is the current approach in the SDK so hopefully we won't have to prioritize any additional work just for this
There was a problem hiding this comment.
I agree with this; being more specific with the requirement while loosening the language a little (MUST -> SHOULD) seems like a good balance.
| 2. The UserKey **MUST** be protected _at rest_. | ||
| 3. The UserKey **MAY** be unprotected while _in use_. | ||
| 4. The UserKey **MUST** be protected while _in transit_. | ||
| 5. The UserKey **MUST NOT** be shared. |
There was a problem hiding this comment.
What about account recovery and emergency access?
| 5. The UserKey **MUST NOT** be shared. | |
| 5. The UserKey **MUST NOT** be shared without _informed and explicit consent_ by the User. |
There was a problem hiding this comment.
You're actually the second person to give me this exact comment so there is definitely room for improvement here. Account recovery and emergency access is not "sharing" according to the definition:
Data sharing
The controlled process in which data leaves the Bitwarden secure environment unprotected. As a consequence the guarantees made by this document will no longer apply. The receiving party may or may not have its own guarantees.
There was a problem hiding this comment.
Interesting. Given that "sharing" has a common use, maybe there is a more specific term that can be used here? (Maybe "export" - or would that be confused with a vault export?)
Alternatively, it's only used a couple of times - maybe you remove the definition and be explicit each time: "The UserKey MUST NOT leave the Bitwarden secure environment in an unprotected state."
As a separate question - does this document have anything to say about when data is exchanged/accessed between users then? EA and Account Recovery are within the Bitwarden secure environment, but it has security implications.
There was a problem hiding this comment.
Both very good suggestions. I think we might need the definition just to make the document easier to read, but you are right that it might be mixed up with vault export. Still think its worth it though, I'll update it when I get a chance!
does this document have anything to say about when data is exchanged/accessed between users then? EA and Account Recovery are within the Bitwarden secure environment, but it has security implications.
Not yet. I think we should redefine this to be "Sharing" and then start writing some principles and requirements on it
| Clients must ensure that once the vault has been locked, no vault data can be accessed in plain | ||
| text, even if the device becomes compromised after the lock occurs. Protections are not guaranteed |
There was a problem hiding this comment.
What about vault data that is intentionally stored in plain text? e.g. modification dates, folder guids, boolean values. Or is this excluded by the definition of "vault data"? (is metadata "sensitive and private information"?)
There was a problem hiding this comment.
Yes, there is actually still an open thread in confluence about this because I was hoping to get Vault involved in classifying data a bit better. My idea is to split up the vault data into groups of varying sensitivity. In that case we might get:
- High: Password
- Moderate: Username
- Low: Creation date
And then say something like "low sensitivity data" may be stored unprotected
There was a problem hiding this comment.
I tweak this a little bit by only grouping the data into highly sensitive and less sensitive (see 528330f) to avoid getting into hard discussions. What do you think?
|
|
||
| ::: | ||
|
|
||
| 1. The authentication tokens MUST be protected at rest if the client provides a mechanism for secure |
There was a problem hiding this comment.
| 1. The authentication tokens MUST be protected at rest if the client provides a mechanism for secure | |
| 1. The authentication tokens **MUST** be protected at rest if the client provides a mechanism for secure |
Suggestion to maintain consistency of font weight.
|
|
||
| 1. The authentication tokens MUST be protected at rest if the client provides a mechanism for secure | ||
| storage. | ||
|
|
There was a problem hiding this comment.
| 2. The authentication tokens **SHOULD** be protected in transit. |
🎟️ Tracking
📔 Objective
This ports over an internal document while making some adjustments to the principles. Requirements will initially be added as-is
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes